Skip to content

Conversation

@mcbarton
Copy link
Collaborator

@mcbarton mcbarton commented Mar 18, 2025

Description

Please include a summary of changes, motivation and context for this PR.

This PR makes general improvements to Emscripten build and ci

Fixes # (issue)

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Requires documentation updates

Testing

Please describe the test(s) that you added and ran to verify your changes.

Checklist

  • I have read the contribution guide recently

@vgvassilev
Copy link
Contributor

These changes are super small. Let's try to put up bigger pull requests that change more things in a more uniform way. Eg. if we have 10 times to improve in the CI let's do them at once instead of generating 10 PRs.

@codecov
Copy link

codecov bot commented Mar 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.46%. Comparing base (a22df96) to head (04d8241).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #533      +/-   ##
==========================================
+ Coverage   75.33%   75.46%   +0.13%     
==========================================
  Files           9        9              
  Lines        3620     3628       +8     
==========================================
+ Hits         2727     2738      +11     
+ Misses        893      890       -3     

see 4 files with indirect coverage changes

see 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mcbarton
Copy link
Collaborator Author

mcbarton commented Mar 18, 2025

These changes are super small. Let's try to put up bigger pull requests that change more things in a more uniform way. Eg. if we have 10 times to improve in the CI let's do them at once instead of generating 10 PRs.

I will do this from now on (I have the very big change in the works to have reusable actions in the workflow to stop repetition). In most cases i'll have to just put the PR title and description as general ci improvements. The reason I put this llvm cache PR (and the other 2) as small incremental changes, was so that if anything went wrong it was obvious. as to what target change or in this case remove of the folder was responsible in case any test broke.

image

The reason I am trying to get small amounts from the cache size is that we are close to the 10GB limit, and currently we won't be able to get the llvm 20 PR in and stay under this limit (its going to be very close). If I cannot get us under this 10GB limit (I'm hoping to just scrape under it), do you have any preference as to what jobs get dropped from the ci? My suggestion would be the llvm 19 Windows job, since we aren't expecting full Windows support until at least llvm 20 I believe.

@mcbarton mcbarton force-pushed the Remove-NATIVE-folder-Emscripten-llvm-build branch from 3c5d2a9 to 1f180a2 Compare March 19, 2025 09:43
@mcbarton mcbarton changed the title Remove NATIVE folder Emscripten llvm build General improvements to Emscripten llvm build Mar 19, 2025
@mcbarton mcbarton force-pushed the Remove-NATIVE-folder-Emscripten-llvm-build branch 2 times, most recently from 98fbe91 to 38030de Compare March 19, 2025 12:53
@mcbarton mcbarton changed the title General improvements to Emscripten llvm build General improvements to Emscripten build and ci Mar 19, 2025
@mcbarton
Copy link
Collaborator Author

@vgvassilev can you please approve this PR? I've added a few improvements to the Emscripten ci as part this PR (detailed in the PR description). If you approve I will merge when activity is quiet in the repo, since it requires me to clear some builds in the Github cache.

@anutosh491
Copy link
Collaborator

Hi,

Mostly looks good. Just a question here and there (afk currently, shall type them out soon )

@mcbarton mcbarton force-pushed the Remove-NATIVE-folder-Emscripten-llvm-build branch 4 times, most recently from 7151916 to ce98219 Compare March 19, 2025 17:33
-DLLVM_INCLUDE_TESTS=OFF \
-DLLVM_ENABLE_THREADS=OFF \
-DLLVM_BUILD_TOOLS=OFF \
-DLLVM_ENABLE_LIBPFM=OFF \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you educate me on what this last flag does and why do we need it ?

Copy link
Collaborator Author

@mcbarton mcbarton Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anutosh491 going by the LLVM documentation

LLVM_ENABLE_LIBPFM:BOOL

    Enable building with libpfm to support hardware counter measurements in LLVM tools. Defaults to ON.

I got the idea for the flag from emsdks build of llvm here https://github.com/emscripten-core/emsdk/blob/b665079cb9ad9eb1704652f962281a7fa1633e2d/emsdk.py#L1124C42-L1124C66 . I thought if its good enough for the developers of Emscripten to think it can be off, then its good enough for CppInterOp.

It also makes sense that if we turn off building the tools, then we should turn off anything which builds stuff to support the tools.

@mcbarton mcbarton requested a review from anutosh491 March 20, 2025 08:14
@mcbarton
Copy link
Collaborator Author

mcbarton commented Mar 20, 2025

@anutosh491 if you approve, please leave it to me to merge this PR, since I need to clear some stuff from the cache first before merging, and it should be done when activity in the repo is quiet.

@anutosh491
Copy link
Collaborator

Actually I just realized that if we build with -DLLVM_BUILD_TOOLS=OFF, it only makes sense to also use CLANG_BUILD_TOOLS=OFF too isn't it ?

@mcbarton
Copy link
Collaborator Author

Actually I just realized that if we build with -DLLVM_BUILD_TOOLS=OFF, it only makes sense to also use CLANG_BUILD_TOOLS=OFF too isn't it ?

@anutosh491 I can do this additional change to the PR. Do you approve the PR subject to this change?

@mcbarton mcbarton force-pushed the Remove-NATIVE-folder-Emscripten-llvm-build branch from ce98219 to 29e808a Compare March 24, 2025 12:28
@mcbarton mcbarton force-pushed the Remove-NATIVE-folder-Emscripten-llvm-build branch from 00d7b4d to 296af5c Compare March 24, 2025 17:57
Copy link
Collaborator

@anutosh491 anutosh491 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Maybe add the clang-enable-tools=off config in the docs too ?

@mcbarton mcbarton merged commit f7ac5b0 into compiler-research:main Mar 26, 2025
72 checks passed
mcbarton added a commit to mcbarton/CppInterOp that referenced this pull request Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants